Add GitHub Enterprise support#3720
Conversation
Enable CDash to post check runs and commit statuses to ghe installations in addition to github.com. Changes: - Add GITHUB_ENTERPRISE_URL config option (env var) - Pass enterprise URL to GitHubClient constructor with apiVersion=null (so it defaults to 'v3') so the PathPrepend plugin builds correct /api/v3/ paths for ghe - Add isGitHubUrl() helpers in GitHub.php and RepositoryUtils.php to match repository URLs against both github.com and the configured GHE host - Convert get_github_api_url() to construct /api/v3/repos/ paths for GHE repositories - Make DoneHandler always create/update the check when a revision exists, rather than requiring the pendingSubmissions recheck flag
| $builder = new GitHubBuilder(); | ||
| $apiClient = new GitHubClient($builder, 'machine-man-preview'); | ||
| $enterpriseUrl = config('cdash.github_enterprise_url'); | ||
| $apiClient = new GitHubClient($builder, null, is_string($enterpriseUrl) ? $enterpriseUrl : null); |
There was a problem hiding this comment.
You should treat an empty string like a null here.
| $enterpriseUrl = config('cdash.github_enterprise_url'); | ||
| if (is_string($enterpriseUrl)) { | ||
| $host = parse_url($enterpriseUrl, PHP_URL_HOST); | ||
| return is_string($host) && str_contains($url, $host); | ||
| } |
There was a problem hiding this comment.
You're referencing the config value multiple times in this file. It would be better to factor that out into a method which returns the enterprise URL or null.
| $repositoryInterface = self::getRepositoryInterface($project); | ||
| try { | ||
| $repositoryInterface = self::getRepositoryInterface($project); | ||
| } catch (Exception $e) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Why is this necessary? This will destroy all exceptions within getRepositoryInterface(), preventing them from making it to the logs.
| // Should we re-run any checks that were previously marked | ||
| // as pending? | ||
| if ($pendingSubmissionsModel !== null && $pendingSubmissionsModel->recheck) { | ||
| $revision = \App\Models\Build::findOrFail((int) $this->Build->Id)->updateStep->revision ?? ''; | ||
| // Create or update the GitHub check for this commit. | ||
| $revision = \App\Models\Build::findOrFail((int) $this->Build->Id)->updateStep->revision ?? ''; | ||
| if ($revision !== '') { |
There was a problem hiding this comment.
What's the thought process here?
| private static function isGitHubUrl(string $url): bool | ||
| { | ||
| if (str_contains($url, 'github.com')) { | ||
| return true; | ||
| } | ||
| $enterpriseUrl = config('cdash.github_enterprise_url'); | ||
| if (is_string($enterpriseUrl)) { | ||
| $host = parse_url($enterpriseUrl, PHP_URL_HOST); | ||
| return is_string($host) && str_contains($url, $host); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This is a duplicate of isGitHubUrl() in app/cdash/app/Lib/Repository/GitHub.php. Let's remove the one in app/cdash/app/Lib/Repository/GitHub.php.
| } | ||
| $enterpriseUrl = config('cdash.github_enterprise_url'); | ||
| if (is_string($enterpriseUrl)) { | ||
| $host = parse_url($enterpriseUrl, PHP_URL_HOST); |
There was a problem hiding this comment.
You can use Laravel's Uri facade to do this more eloquently (note: untested).
Apply throughout.
| $host = parse_url($enterpriseUrl, PHP_URL_HOST); | |
| $host = Uri::of($enterpriseUrl)->host(); |
| $enterpriseUrl = config('cdash.github_enterprise_url'); | ||
| if (is_string($enterpriseUrl)) { | ||
| $host = parse_url($enterpriseUrl, PHP_URL_HOST); | ||
| if (is_string($host)) { | ||
| $idx = strpos($github_url, $host); | ||
| if ($idx !== false) { | ||
| // For GHE, ...://<host>/<user>/<repo> becomes ...://<host>/api/v3/repos/<user>/<repo> | ||
| $idx2 = $idx + strlen($host) + 1; | ||
| $api_url = substr($github_url, 0, $idx) . $host . '/api/v3/repos/'; | ||
| $api_url .= substr($github_url, $idx2); | ||
| return $api_url; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
A few questions/comments:
- Can you please add a comment explaining what's going on here?
- Does
...<host>/api/v3...work for GitHub.com too? i.e., are we strictly required to have a GHE-specific code path here? - Assuming we need a GHE-specific code path, can you wrap the following code in an
elseso it's clear that there are two branches? It took a second for me to understand what was going on here. - There are probably Laravel string facade methods which could make this cleaner.
|
@arhowe00 Note that CDash squashes commits, so your PR description will be included in the git history permanently. Please leave any questions/comments as comments on GitHub and treat the PR description as the commit message. |
This change makes it easier to configure GitHub integration for users who may use a private GHE instance. CDash's GitHub integration (check runs, commit statuses, PR comments) is currently hardcoded to api.github.com.
Could the reviewer look at (1) how I change GitHubClient as well as (2) whether these changes are reasonable?
(1) You can see that constructing with
nullas the api version sets it to 'v3' which is the version that the only enterprise client I'm aware of uses. Why is it currently'machine-man-preview'?(2) I changed this so checks are always posted if there is a revision. It seems this flag is now read-only. I've added another commit to guard createOrUpdateCheck, since
if ($revision !== '')fires for any project regardless of whether it's actually configured with GHE